🏡 feat: Add Hosted App Runner - #57
Conversation
7aaab6c to
6c71834
Compare
|
@codex review Please review the current PR head 6c71834. Confirm that this exact commit is the reviewed commit and ignore findings that apply only to the pre-restack head. This branch was semantically rebased onto current main adf2adf; the Docker conflict was resolved by retaining both the newer local-oci-runtime target and this PR’s independent lambda-microvm-app-host target. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c71834834
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| (req: Request, res: Response, next: NextFunction) => { | ||
| const failure = bindSessionFromHeader(req); | ||
| if (failure) return res.status(failure.status).json(failure.body); | ||
| return hostedAppSupervisor.start(req.body) |
There was a problem hiding this comment.
Block checkpoint restores while an app is running
After this call returns, the existing POST /session/restore route remains available for the same session and can run without coordinating with hostedAppSupervisor. A retried or concurrently issued restore renames the live workspace, installs a replacement, and recursively deletes the old tree while the resident process still has its cwd and open files there, so the app can continue against an unlinked revision or fail unpredictably. Reject restores while the supervisor is starting/running, or stop and serialize the app around restoration.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2af7947. Hosted-app runners now execute checkpoint restore through the supervisor’s transition queue, and the mutation is rejected with a bounded 409 while an app is starting, running, or stopping. The supervisor regression verifies the workspace operation never runs while active.
| if (await this.deps.probePort(config.hosted_app_port)) { | ||
| active.status.state = 'running'; |
There was a problem hiding this comment.
Recheck child state after awaiting the readiness probe
If the tracked process emits exit or error while probePort() is pending, the handler marks it failed and queues cgroup cleanup, but a successful probe then unconditionally overwrites that state with running and returns 200. This can happen when a process crashes just after opening the port or leaves a briefly listening descendant; the queued cleanup subsequently kills the cgroup while status continues to report running. Recheck that active.process is still the child and the state is not failed after the awaited probe before declaring readiness.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2af7947. After every awaited port probe, start now rechecks both the tracked child identity and failed state before publishing running. A deterministic test exits the child while the successful probe is pending and verifies start fails.
| '/bin/bash', | ||
| path.join(runtime.pkgdir, 'run'), | ||
| workspace.entrypoint, |
There was a problem hiding this comment.
Provide the workspace path expected by JavaScript runtimes
The Node and Bun runtime launchers in docker/package-init.sh install their curated packages by linking them at /mnt/data/node_modules, which works only because NsJail normally bind-mounts the workspace at /mnt/data. Hosted apps are spawned directly with the real workspace as cwd, so /mnt/data is not that workspace and the unprivileged launcher cannot create the link; consequently Node ESM apps using the bundled packages fail with ERR_MODULE_NOT_FOUND unless the restored checkpoint happens to contain its own node_modules link. Set up the runtime package link in the actual hosted workspace (or provide the equivalent mount) before dropping privileges.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2af7947. The supervisor now prepares the actual session workspace with a node_modules link to the selected runtime’s curated packages before dropping privileges. A filesystem regression verifies the link resolves to the runtime package tree.
| 409, | ||
| ); | ||
| } | ||
| await this.stopImpl(); |
There was a problem hiding this comment.
Validate a replacement before stopping the active app
When a healthy app is active, any request with a different revision reaches stopImpl() before the runtime is resolved or the cwd and entrypoint are checked. A deployment typo such as an unavailable runtime or missing entrypoint therefore terminates the serving revision and then returns a 400 without launching anything, even though those validation checks do not require freeing the port or cgroup. Resolve the session/runtime and validate workspace paths before stopping the current process so an invalid replacement cannot cause avoidable downtime.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2af7947. Runtime resolution and workspace cwd/entrypoint validation now happen before stopping the serving revision, followed by authoritative path revalidation after the old cgroup is gone. The replacement regression confirms an unavailable runtime leaves the active app running and sends no signal.
|
@codex review Please review the current PR head 2af7947. Confirm that this exact commit is the reviewed commit and ignore findings that apply only to earlier heads. All four findings are addressed as one lifecycle invariant: workspace mutation and replacement preparation serialize with the resident process; readiness is revalidated after awaits; JavaScript packages are linked into the actual hosted workspace. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2af7947eb2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| await this.deps.prepareRuntimeWorkspace(ownership.dir, runtime); | ||
| await this.stopImpl(); |
There was a problem hiding this comment.
Move package linking until after the resident app stops
When replacing a running revision whose workspace lacks node_modules—for example, a Python-to-Node replacement—this newly added call creates the symlink before stopImpl() terminates the resident process. Because that process shares and can modify the workspace, it can observe, delete, or replace the link before its cgroup is gone; the post-stop revalidation checks only the cwd and entrypoint, so the replacement can launch without the intended package tree. The fresh evidence in this head is the ordering of prepareRuntimeWorkspace() immediately before stopImpl(); keep any non-mutating preflight before the stop, but perform the workspace mutation afterward.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a528185. Replacement preflight remains non-mutating, then the old app is stopped and its cgroup confirmed empty before the runtime package link is created. Workspace paths are revalidated after that mutation.
| try { | ||
| await fsp.lstat(workspaceModules); | ||
| return; |
There was a problem hiding this comment.
Refresh supervisor-managed links when changing runtimes
When a workspace already contains the symlink created for an earlier Node or Bun revision, replacing that revision with another JavaScript runtime or version reaches this unconditional return without verifying the link target. The new process consequently resolves bare ESM imports through the previous runtime's package tree instead of the selected runtime's curated packages, which can expose different versions or incompatible runtime-specific packages. The fresh evidence in this head is the new helper's treatment of every existing path as user-owned; preserve real user directories, but recognize and update symlinks pointing into managed runtime package trees.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a528185. The workspace helper now preserves user-owned directories and external symlinks, but recognizes links targeting the configured managed package tree and atomically refreshes them for the selected runtime. The regression switches from Node to Bun package roots.
| } | ||
| /* Always sweep the cgroup: the tracked parent may have exited cleanly | ||
| * while a daemonized descendant stayed alive in a different process group. */ | ||
| await this.deps.killCgroup(); |
There was a problem hiding this comment.
Wait for cgroup emptiness before publishing stopped
When the tracked parent exits after daemonizing a descendant, the final killCgroup() only queues SIGKILL; this method then immediately clears active and lets a serialized workspace restore proceed even though the descendant may not have exited yet. That descendant can still finish an in-flight workspace operation while restore renames or removes the tree, recreating the race the restore serialization is intended to prevent. Fresh evidence in this head is that prepareHostedAppCgroup() explicitly waits for cgroup.events to report populated 0, while the stop path omits that confirmation; reuse the same bounded emptiness wait before reporting stopped.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a528185. killHostedAppCgroup now polls cgroup.events until populated is 0, bounded by HOSTED_APP_STOP_TIMEOUT_MS, before stop can publish stopped or allow a workspace mutation/replacement to continue. A deterministic test holds cleanup and verifies stop remains pending.
| void this.serialize(async () => { | ||
| if (active.status.state === 'stopped') return; | ||
| await this.deps.killCgroup(); |
There was a problem hiding this comment.
Skip stale exit cleanup after a replacement starts
If a replacement already owns the transition queue and the old child exits during one of its awaited preflight operations, this exit handler queues cleanup behind that replacement. stopImpl() then sees the old child as absent, sweeps its cgroup without changing the old status from failed, and launches the new revision; after the replacement releases the queue, this closure still passes its old-status check and kills the shared cgroup containing the newly reported-ready process. The fresh interleaving missed by the existing regression test is that the exit happens during replacement preflight rather than before start() is called; skip cleanup when this.active !== active or mark the old cleanup as consumed by stopImpl().
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a528185. Queued exit cleanup is now instance-fenced with this.active === active, and stopImpl marks the no-parent case stopped after the cgroup has drained. The regression exits the old child during replacement preflight and verifies the replacement remains running without a second stale cgroup kill.
|
@codex review exact head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a528185203
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const state = this.active?.status.state; | ||
| if (state === 'starting' || state === 'running' || state === 'stopping') { |
There was a problem hiding this comment.
Reconfirm failed apps' cgroups before mutating workspaces
If the deferred cleanup after an unexpected exit rejects—for example, because cgroup.events remains populated until the timeout—the rejection is only logged and the app remains in failed. This check then permits a subsequent checkpoint restore without retrying killCgroup(), so a daemonized descendant that caused the cleanup failure can still access the workspace while restore renames and deletes it. The fresh evidence in this head is that the cleanup rejection leaves no confirmed-empty marker; require successful cgroup cleanup for failed apps before invoking the mutation, or keep rejecting the restore.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ea267e8. Active app state now explicitly tracks whether the shared cgroup has been confirmed drained. A workspace mutation retries cleanup for a failed app and fails closed with hosted_app_cleanup_failed if that drain cannot be proven; the regression verifies no mutation occurs until cleanup later succeeds.
| if ( | ||
| this.active | ||
| && this.active.request.app_id === request.app_id | ||
| && this.active.request.revision === request.revision | ||
| && this.active.specKey !== specKey | ||
| ) { |
There was a problem hiding this comment.
Preserve revision immutability after stop or replacement
This conflict check compares the request only with the currently active app, but stopImpl() discards that record and a replacement overwrites it. Consequently, after stopping a revision—or after temporarily running another revision—a caller can reuse the original app_id and revision with different arguments, environment, runtime, or entrypoint and the supervisor launches it successfully, despite the API contract requiring changed launch settings to use a new revision. Retain the spec key for previously observed app revisions for the lifetime of the VM/session rather than enforcing immutability only while that revision is active.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ea267e8. The supervisor retains a bounded, session-lifetime registry keyed by app_id/revision with SHA-256 fingerprints of canonical launch specs. Reuse with changed settings is rejected after stop and after another revision replaces it, while an exact stopped revision remains restartable.
|
@codex review exact head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea267e8821
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (failure) return res.status(failure.status).json(failure.body); | ||
| return hostedAppSupervisor.stop() | ||
| .then(status => status ? res.status(200).json(status) : res.status(204).send()) | ||
| .catch(next); |
There was a problem hiding this comment.
Return a server error when stop cleanup fails
When cgroup cleanup rejects—for example, because it remains populated past the configured timeout—hostedAppSupervisor.stop() rejects and this forwards the plain error to the global handler, whose fallback status is 400. That misclassifies a transient runner-side cleanup failure as a client error, so the control plane may treat it as non-retryable instead of recycling or retrying the host; translate cleanup failures to a 5xx HostedAppError, as the workspace-mutation path does.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed at current rebased head ea97f37. Public stop now translates runner-side cleanup failures into hosted_app_cleanup_failed (503), and the stop route uses the hosted-app error mapper. A supervisor regression verifies the retryable server classification.
| return restoreSessionCheckpoint(req, res).catch(next); | ||
| const restore = (): Promise<void> => restoreSessionCheckpoint(req, res); | ||
| return (config.hosted_apps_enabled | ||
| ? hostedAppSupervisor.withWorkspaceMutation(restore) |
There was a problem hiding this comment.
Coordinate checkpoint reads with the hosted app
The hosted-mode coordination added here covers only checkpoint restore; GET /session/checkpoint still directly tars the live workspace while a resident app may be writing it. During a running or incompletely cleaned-up app, GNU tar can abort with “file changed as we read it,” or it can successfully capture mutually inconsistent versions of files changed via atomic renames, producing an invalid recovery point. Reject or serialize checkpoint creation while the app can access the workspace, just as this path does for restore.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed at current rebased head ea97f37. Both checkpoint creation and restore now run through the same serialized quiesced-workspace gate in hosted mode. The route regression verifies GET /session/checkpoint returns the workspace-busy conflict instead of reading while an app is active.
| } | ||
| const env: NodeJS.ProcessEnv = { | ||
| ...callerEnv, | ||
| ...runtime.env_vars, |
There was a problem hiding this comment.
Preserve packaged runtimes for Bash-hosted apps
When language resolves to the supported Bash runtime, this environment contains only Bash's system-level PATH. Ordinary Bash executions call aggregateBashExtras() to prepend every installed runtime package directory and create the preferred node_modules link, allowing shell entrypoints to invoke the curated Python, Node, or Bun runtimes; the hosted path skips both steps, so common Bash servers that delegate to those tools either get command not found, use an unrelated system binary, or cannot resolve bundled JavaScript packages. Apply the same Bash aggregation/link preparation here or reject Bash as unsupported.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed at current rebased head ea97f37. Bash hosted apps now reuse aggregateBashExtras across the installed runtime registry, receive curated PATH/NODE_PATH entries, and prepare the preferred managed node_modules link in the real workspace. The regression verifies the launch environment and link target.
ea267e8 to
ea97f37
Compare
Summary
lambda-microvm-app-hostimage and feature-gated resident-server control API/executework on app-host images and bind every control operation to the authenticated runtime sessionBoundary
This is the Lambda resident-server adapter and image/runtime contract. The fleet-level app lease registry and owner-authenticated preview gateway are delivered by stacked PR #58; raw AWS endpoints/tokens are explicitly not exposed by this PR.
Verification
env -u AWS_CA_BUNDLE bun testinapi— 366 pass, 16 skipped, 0 failedbash -nfor both Lambda artifact builder and hosted-app launcher7aaab6cbun testwithout unsetting the workstation-providedAWS_CA_BUNDLEmakes three pre-existing hardened-startup tests intentionally reject that inherited secret-like AWS environment variable; the clean-env run above passes the full suite.